Convert GeoPointMapActivity to Fragment#7253
Conversation
seadowg
force-pushed
the
geopoint-fragment
branch
from
89e5e17 to
eb1223a
Compare
seadowg
changed the title
seadowg
changed the title
GeoPointMapActivity to Fragment
|
Tested with success! Verified on a device with Android 10 Verified cases:
|
|
Tested with success Verified on Android 16 |
|
|
||
| object BundleExt { | ||
|
|
||
| inline fun <reified T : Parcelable> Bundle.getParcelableExtraCompat(name: String): T? { |
There was a problem hiding this comment.
The name getParcelableExtraCompat is a bit misleading here - Extra implies an Intent (as in getXxxExtra), but this operates on a Bundle, which has no extras. Suggest renaming to getParcelableCompat.
| import android.os.Parcelable | ||
| import androidx.core.os.BundleCompat | ||
|
|
||
| object BundleExt { |
There was a problem hiding this comment.
Wrapping a single extension function in an object adds an extra layer without much benefit. A top-level extension function would be more idiomatic and just as easy to import.
| import org.odk.collect.geo.items.MappableItem | ||
| import org.odk.collect.maps.MapPoint | ||
| import org.odk.collect.testshared.FakeScheduler | ||
| import org.odk.collect.testshared.getOrAwaitValue |
There was a problem hiding this comment.
There are many unused imports here. Why did all checks pass?
| import org.odk.collect.android.widgets.interfaces.GeoDataRequester | ||
| import org.odk.collect.android.widgets.utilities.BindAttributes.ALLOW_MOCK_ACCURACY | ||
| import org.odk.collect.androidshared.ui.DialogFragmentUtils | ||
| import org.odk.collect.geo.Constants.EXTRA_DRAGGABLE_ONLY |
There was a problem hiding this comment.
This const is unused now, and we can remove it.
| placeMarkerButton!!.isEnabled = false | ||
|
|
||
| captureLocation = true | ||
| pointFromIntent = true |
There was a problem hiding this comment.
It doesn't come from an intent anymore so we should update the naming here.
| } | ||
|
|
||
| private fun cancel() { | ||
| getParentFragmentManager().setFragmentResult(REQUEST_GEOPOINT, Bundle.EMPTY) |
There was a problem hiding this comment.
You can use the property parentFragmentManager.setFragmentResult(REQUEST_GEOPOINT, Bundle.EMPTY).
| setView(this, R.layout.geopoint_layout, false); | ||
| } catch (NoClassDefFoundError e) { | ||
| Timber.e(e, "Google maps not accessible due to: %s ", e.getMessage()); | ||
| ToastUtils.showShortToast(org.odk.collect.strings.R.string.google_play_services_error_occured); |
There was a problem hiding this comment.
This disappeared. Don't we need it anymore?
| import org.odk.collect.testshared.getOrAwaitValue | ||
|
|
||
| @RunWith(AndroidJUnit4::class) | ||
| class GeoPointMapDialogFragmentTest { |
There was a problem hiding this comment.
GeoPointMapDialogFragmentTest only covers the input configuration (configures GeoPointMapFragment with answer/draggable/readOnly), but none of the result-handling logic in onCreateFragment/onAnswer is tested. Compared to the sibling GeoPolyDialogFragmentTest there are some cases missing.
4 participants
Work towards #7118
Why is this the best possible solution? Were any other approaches considered?
We ideally want to be showing the geo point map UI within
FormFillingActivityso that we don't have to deal with passing reference geometry between Activity objects. Like with geopoly questions, I've converted it to aDialogFragment.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
This significantly restructures the way geopoint with
maps/placement-mapsworks, so we'll want to test this feature thoroughly. No changes to how maps work has been made, so this only needs to be tested with one source/style.Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest